Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Reflection-based cloning #13432

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 20, 2024

Objective

Using Reflect::clone_value can be somewhat confusing to those unfamiliar with how Bevy's reflection crate works. For example take the following code:

let value: usize = 123;
let clone: Box<dyn Reflect> = value.clone_value();

What can we expect to be the underlying type of clone? If you guessed usize, then you're correct! Let's try another:

#[derive(Reflect, Clone)]
struct Foo(usize);

let value: Foo = Foo(123);
let clone: Box<dyn Reflect> = value.clone_value();

What about this code? What is the underlying type of clone? If you guessed Foo, unfortunately you'd be wrong. It's actually DynamicStruct.

It's not obvious that the generated Reflect impl actually calls Struct::clone_dynamic under the hood, which always returns DynamicStruct.

There are already some efforts to make this a bit more apparent to the end-user: #7207 changes the signature of Reflect::clone_value to instead return Box<dyn PartialReflect>, signaling that we're potentially returning a dynamic type.

But why can't we return Foo?

Foo can obviously be cloned— in fact, we already derived Clone on it. But even without the derive, this seems like something Reflect should be able to handle. Almost all types that implement Reflect either contain no data (trivially clonable), they contain a #[reflect_value] type (which, by definition, must implement Clone), or they contain another Reflect type (which recursively fall into one of these three categories).

This PR aims to enable true reflection-based cloning where you get back exactly the type that you think you do.

Solution

Add a Reflect::reflect_clone method which returns Result<Box<dyn Reflect>, ReflectCloneError>, where the Box<dyn Reflect> is guaranteed to be the same type as Self.

#[derive(Reflect)]
struct Foo(usize);

let value: Foo = Foo(123);
let clone: Box<dyn Reflect> = value.reflect_clone().unwrap();
assert!(clone.is::<Foo>());

Notice that we didn't even need to derive Clone for this to work: it's entirely powered via reflection!

Under the hood, the macro generates something like this:

fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Self {
        // The `reflect_clone` impl for `usize` just makes use of its `Clone` impl
        0: Reflect::reflect_clone(&self.0)?.take().map_err(/* ... */)?,
    }))
}

If we did derive Clone, we can tell Reflect to rely on that instead:

#[derive(Reflect, Clone)]
#[reflect(Clone)]
struct Foo(usize);
Generated Code
fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Clone::clone(self)))
}

Or, we can specify our own cloning function:

#[derive(Reflect)]
#[reflect(Clone(incremental_clone))]
struct Foo(usize);

fn incremental_clone(value: &usize) -> usize {
  *value + 1
}
Generated Code
fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(incremental_clone(self)))
}

Similarly, we can specify how fields should be cloned. This is important for fields that are #[reflect(ignore)]'d as we otherwise have no way to know how they should be cloned.

#[derive(Reflect)]
struct Foo {
 #[reflect(ignore, clone)]
  bar: usize,
  #[reflect(ignore, clone = "incremental_clone")]
  baz: usize,
}

fn incremental_clone(value: &usize) -> usize {
  *value + 1
}
Generated Code
fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> {
    Ok(Box::new(Self {
        bar: Clone::clone(&self.bar),
        baz: incremental_clone(&self.baz),
    }))
}

If we don't supply a clone attribute for an ignored field, then the method will automatically return Err(ReflectCloneError::FieldNotClonable {/* ... */}).

Err values "bubble up" to the caller. So if Foo contains Bar and the reflect_clone method for Bar returns Err, then the reflect_clone method for Foo also returns Err.

Attribute Syntax

You might have noticed the differing syntax between the container attribute and the field attribute.

This was purely done for consistency with the current attributes. There are PRs aimed at improving this. #7317 aims at making the "special-cased" attributes more in line with the field attributes syntactically. And #9323 aims at moving away from the stringified paths in favor of just raw function paths.

Compatibility with Unique Reflect

This PR was designed with Unique Reflect (#7207) in mind. This method actually wouldn't change that much (if at all) under Unique Reflect. It would still exist on Reflect and it would still Option<Box<dyn Reflect>>. In fact, Unique Reflect would only improve the user's understanding of what this method returns.

We may consider moving what's currently Reflect::clone_value to PartialReflect and possibly renaming it to partial_reflect_clone or clone_dynamic to better indicate how it differs from reflect_clone.

Testing

You can test locally by running the following command:

cargo test --package bevy_reflect

Changelog

  • Added Reflect::reflect_clone method
  • Added ReflectCloneError error enum
  • Added #[reflect(Clone)] container attribute
  • Added #[reflect(clone)] field attribute

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible S-Blocked This cannot move forward until something else changes A-Reflection Runtime information about types D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 20, 2024
@MrGVSV MrGVSV added this to the 0.15 milestone May 20, 2024
fn parse_clone(&mut self, input: ParseStream) -> syn::Result<()> {
let ident = input.parse::<kw::Clone>()?;

if input.peek(token::Paren) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should use darling as a helper crate to parse attributes?
It seems easier than what you're doing, especially if the attributes become more complex later on

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could utilize something like darling in the future (not this PR though). I'm not sure our parsing logic is so complex we really need it, but it's certainly worth considering.

@@ -509,6 +533,24 @@ impl ContainerAttributes {
}
}

pub fn get_clone_impl(&self, bevy_reflect_path: &Path) -> Option<proc_macro2::TokenStream> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used? I couldn't understand this part.
I get the clone_impl on struct,tuple,enum,value,etc. but not this.

Or is this the top-level impl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used by derive_data.rs. I could probably move this logic into that file directly, since it's the only place where it's used.

Copy link
Contributor

@cBournhonesque cBournhonesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I like the change!
I think the naming will be clearer when the PartialReflect/UniqueReflect PR gets merged as well.
Had some small comments, and i'll probably wait for the blocking PR to be merged before approving

@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/reflect-clone branch from 0a05f93 to 51f9b2d Compare May 22, 2024 23:06
@MrGVSV MrGVSV added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels May 22, 2024
@MrGVSV MrGVSV force-pushed the mrgvsv/reflect/reflect-clone branch from 51f9b2d to ad1146e Compare May 22, 2024 23:09
@MrGVSV MrGVSV requested a review from cBournhonesque May 22, 2024 23:09
@MrGVSV MrGVSV marked this pull request as ready for review May 22, 2024 23:09
github-merge-queue bot pushed a commit that referenced this pull request Sep 15, 2024
# Objective

Fix #10284.

## Solution

When `DynamicSceneBuilder` extracts entities, they are cloned via
`PartialReflect::clone_value`, making them into dynamic versions of the
original components. This loses any custom `ReflectSerialize` type data.
Dynamic scenes are deserialized with the original types, not the dynamic
versions, and so any component with a custom serialize may fail. In this
case `Rect` and `Vec2`. The dynamic version includes the field names 'x'
and 'y' but the `Serialize` impl doesn't, hence the "expect float"
error.

The solution here: Instead of using `clone_value` to clone the
components, `FromReflect` clones and retains the original information
needed to serialize with any custom `Serialize` impls. I think using
something like `reflect_clone` from
(#13432) might make this more
efficient.

I also did the same when deserializing dynamic scenes to appease some of
the round-trip tests which use `ReflectPartialEq`, which requires the
types be the same and not a unique/proxy pair. I'm not sure it's
otherwise necessary. Maybe this would also be more efficient when
spawning dynamic scenes with `reflect_clone` instead of `FromReflect`
again?

An alternative solution would be to fall back to the dynamic version
when deserializing `DynamicScene`s if the custom version fails. I think
that's possible. Or maybe simply always deserializing via the dynamic
route for dynamic scenes?

## Testing

This example is similar to the original test case in #10284:

``` rust
#![allow(missing_docs)]

use bevy::{prelude::*, scene::SceneInstanceReady};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, (save, load).chain())
        .observe(check)
        .run();
}

static SAVEGAME_SAVE_PATH: &str = "savegame.scn.ron";

fn save(world: &mut World) {
    let entity = world.spawn(OrthographicProjection::default()).id();

    let scene = DynamicSceneBuilder::from_world(world)
        .extract_entity(entity)
        .build();

    if let Some(registry) = world.get_resource::<AppTypeRegistry>() {
        let registry = registry.read();
        let serialized_scene = scene.serialize(&registry).unwrap();
        // println!("{}", serialized_scene);
        std::fs::write(format!("assets/{SAVEGAME_SAVE_PATH}"), serialized_scene).unwrap();
    }

    world.entity_mut(entity).despawn_recursive();
}

fn load(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(DynamicSceneBundle {
        scene: asset_server.load(SAVEGAME_SAVE_PATH),
        ..default()
    });
}

fn check(_trigger: Trigger<SceneInstanceReady>, query: Query<&OrthographicProjection>) {
    dbg!(query.single());
}
```


## Migration Guide

The `DynamicScene` format is changed to use custom serialize impls so
old scene files will need updating:

Old: 

```ron
(
  resources: {},
  entities: {
    4294967299: (
      components: {
        "bevy_render::camera::projection::OrthographicProjection": (
          near: 0.0,
          far: 1000.0,
          viewport_origin: (
            x: 0.5,
            y: 0.5,
          ),
          scaling_mode: WindowSize(1.0),
          scale: 1.0,
          area: (
            min: (
              x: -1.0,
              y: -1.0,
            ),
            max: (
              x: 1.0,
              y: 1.0,
            ),
          ),
        ),
      },
    ),
  },
)
```

New:

```ron
(
  resources: {},
  entities: {
    4294967299: (
      components: {
        "bevy_render::camera::projection::OrthographicProjection": (
          near: 0.0,
          far: 1000.0,
          viewport_origin: (0.5, 0.5),
          scaling_mode: WindowSize(1.0),
          scale: 1.0,
          area: (
            min: (-1.0, -1.0),
            max: (1.0, 1.0),
          ),
        ),
      },
    ),
  },
)
```

---------

Co-authored-by: Gino Valente <[email protected]>
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good PR, I just have some small questions :D

Comment on lines +359 to +365
return #FQResult::Err(
#bevy_reflect_path::ReflectCloneError::FieldNotClonable {
field: #field_id,
variant: #FQOption::Some(::std::borrow::Cow::Borrowed(#variant_name)),
container_type_path: ::std::borrow::Cow::Borrowed(<Self as #bevy_reflect_path::TypePath>::type_path())
}
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood it right, if we have an ignored field, we can only fallback to Derive impl, right?

@@ -4,15 +4,15 @@ use bevy_reflect_derive::{impl_reflect, impl_reflect_value};
use glam::*;

impl_reflect!(
#[reflect(Debug, Hash, PartialEq, Default)]
#[reflect(Clone, Debug, Hash, PartialEq, Default)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should we use #[reflect(Clone)] Whenever possible?

/// Unlike [`Reflect::clone_value`], which often returns a dynamic representation of `Self`,
/// this method attempts create a clone of `Self` directly, if possible.
///
/// If the clone cannot be performed, `None` is returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we must warn the user which cases can return None
From what I read in this PR, this returns None if Clone isn't implemented for all types, but when I first read the PR description, this left me very confused: How can Clone fail?

@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Oct 8, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
@eugineerd eugineerd mentioned this pull request Dec 8, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 18, 2024
# Objective

#16132 introduced entity cloning functionality, and while it works and
is useful, it can be made faster. This is the promised follow-up to
improve performance.

## Solution

**PREFACE**: This is my first time writing `unsafe` in rust and I have
only vague idea about what I'm doing. I would encourage reviewers to
scrutinize `unsafe` parts in particular.

The solution is to clone component data to an intermediate buffer and
use `EntityWorldMut::insert_by_ids` to insert components without
additional archetype moves.

To facilitate this, `EntityCloner::clone_entity` now reads all
components of the source entity and provides clone handlers with the
ability to read component data straight from component storage using
`read_source_component` and write to an intermediate buffer using
`write_target_component`. `ComponentId` is used to check that requested
type corresponds to the type available on source entity.

Reflect-based handler is a little trickier to pull of: we only have
`&dyn Reflect` and no direct access to the underlying data.
`ReflectFromPtr` can be used to get `&dyn Reflect` from concrete
component data, but to write it we need to create a clone of the
underlying data using `Reflect`. For this reason only components that
have `ReflectDefault` or `ReflectFromReflect` or `ReflectFromWorld` can
be cloned, all other components will be skipped. The good news is that
this is actually only a temporary limitation: once #13432 lands we will
be able to clone component without requiring one of these `type data`s.

This PR also introduces `entity_cloning` benchmark to better compare
changes between the PR and main, you can see the results in the
**showcase** section.

## Testing

- All previous tests passing
- Added test for fast reflect clone path (temporary, will be removed
after reflection-based cloning lands)
- Ran miri

## Showcase
Here's a table demonstrating the improvement:

| **benchmark** | **main, avg** | **PR, avg** | **change, avg** |
| ----------------------- | ------------- | ----------- |
--------------- |
| many components reflect | 18.505 µs | 2.1351 µs | -89.095% |
| hierarchy wide reflect* | 22.778 ms | 4.1875 ms | -81.616% |
| hierarchy tall reflect* | 107.24 µs | 26.322 µs | -77.141% |
| hierarchy many reflect | 78.533 ms | 9.7415 ms | -87.596% |
| many components clone | 1.3633 µs | 758.17 ns | -45.937% |
| hierarchy wide clone* | 2.7716 ms | 3.3411 ms | +20.546% |
| hierarchy tall clone* | 17.646 µs | 20.190 µs | +17.379% |
| hierarchy many clone | 5.8779 ms | 4.2650 ms | -27.439% |

*: these benchmarks have entities with only 1 component

## Considerations
Once #10154 is resolved a large part of the functionality in this PR
will probably become obsolete. It might still be a little bit faster
than using command batching, but the complexity might not be worth it.

## Migration Guide
- `&EntityCloner` in component clone handlers is changed to `&mut
ComponentCloneCtx` to better separate data.
- Changed `EntityCloneHandler` from enum to struct and added convenience
functions to add default clone and reflect handler more easily.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Chris Russell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants